Skip to content

feat: password protected stores#3276

Open
vlaux wants to merge 17 commits intodevfrom
feat/password-protection
Open

feat: password protected stores#3276
vlaux wants to merge 17 commits intodevfrom
feat/password-protection

Conversation

@vlaux
Copy link
Copy Markdown

@vlaux vlaux commented Apr 15, 2026

FAS-930

What's the purpose of this pull request?

Store deployments frequently expose preview URLs to external audiences during development and testing phases. This creates multiple security and usability concerns:

  • SEO Indexing Issues: Search engines can index preview URLs, leading to duplicate content and rankings degradation
  • Consumer Confusion: Customers may attempt to purchase from preview URLs instead of production stores
  • Information Disclosure: Unreleased products, features, or sensitive configurations may become visible to unintended audiences
  • Operational Friction: Commerce developers currently require manual password protection setup at provider level (e.g., Vercel), creating implementation burden

How it works?

Merchants set up password protection settings to their builds, which can be applied to default domains (*.vtex.app) or custom domains (mystore.com).

How to test it?

Password protection setup on WebOps: https://faststoreplatform3.myvtex.com/admin/webops/deployments
Running store: https://sfj-ae50243--faststoreplatform3.preview.vtex.app/ (password is 123)

Starters Deploy Preview

References

RFC: https://docs.google.com/document/d/1rOwddxX8V08cY-lhJR6VqQ1g-oznJ1dBuNMTCJtO7FI/edit?tab=t.ftdefyeg4z3l#heading=h.n2j25ht9xpmm

Checklist

You may erase this after checking them all 😉

PR Title and Commit Messages

  • PR title and commit messages follow the Conventional Commits specification
    • Available prefixes: feat, fix, chore, docs, style, refactor, ci and test

PR Description

  • Added a label according to the PR goal - breaking change, bug, contributing, performance, documentation..

Dependencies

  • Committed the pnpm-lock.yaml file when there were changes to the packages

Documentation

  • PR description
  • For documentation changes, ping @Mariana-Caetano to review and update (Or submit a doc request)

Summary by CodeRabbit

  • New Features

    • Added password protection authentication with dedicated login page
    • Implemented middleware-based request authentication with JWT token verification and automatic session renewal
    • Added password authentication API endpoint with secure cookie management
  • Tests

    • Added comprehensive test coverage for authentication services and cookie utilities
  • Chores

    • Removed disabled middleware code and related activation logic

vlaux added 5 commits April 9, 2026 07:53
Introduces AuthenticationService that handles the full password
protection flow: JWT RS256 verification via jose, session renewal,
negative caching, and domain-based protection scope. Includes
fail-closed behavior for default domains and fail-open for custom
domains to protect production reliability.

Adds jose dependency for Edge Runtime-compatible JWT verification.

Made-with: Cursor
…ditional redirects

- Rename middleware__DISABLED.ts to middleware.ts (middleware always active)
- Integrate AuthenticationService for password protection flow
- Run redirects only when ENABLE_REDIRECTS_MIDDLEWARE is set at runtime
- Remove middleware renaming logic from CLI generate.ts

Made-with: Cursor
…meouts

Add webops-api helpers for public-key, status, session, and renew endpoints.
Use them from AuthenticationService and the fs auth login route; normalize
WEBOPS_API_URL and encode storeId on status queries. Improve login page a11y.
@codesandbox-ci
Copy link
Copy Markdown

codesandbox-ci Bot commented Apr 15, 2026

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 15, 2026

Walkthrough

This PR removes CLI-based redirects middleware activation and introduces a comprehensive password-protection authentication system featuring Next.js middleware, JWT verification via jose, session renewal, and a login API endpoint with UI for FastStore applications.

Changes

Cohort / File(s) Summary
CLI Generation
packages/cli/src/utils/generate.ts
Removed ENABLE_REDIRECTS_MIDDLEWARE constant and enableRedirectsMiddleware() function that previously toggled middleware activation during build generation.
Core Dependencies
packages/core/package.json
Added jose (^6.1.3) runtime dependency for JWT verification and cryptographic operations.
Middleware & Authentication
packages/core/src/middleware.ts, packages/core/src/server/authentication-service.ts
Implemented Next.js middleware with route matching config and a new AuthenticationService that performs JWT verification, session renewal, WebOps-based protection-status checks, cookie management, and redirect-to-login enforcement based on domain scope and protection state.
API Login & UI
packages/core/src/pages/api/fs/auth/login.ts, packages/core/src/pages/fs-auth-login/index.tsx, packages/core/src/pages/fs-auth-login/fs-auth-login.module.scss
Added password-authenticated login API route with upstream session validation, token/cookie issuance, and redirect-URL sanitization; paired with client-side login page component and layout styling.
Auth Utilities
packages/core/src/server/password-protection/auth-cookie.ts, packages/core/src/server/password-protection/webops-api.ts
Added helpers for secure cookie determination (Secure flag logic based on environment/host), and centralized WebOps API endpoint construction with timeout configuration.
Disabled Middleware Cleanup
packages/core/src/middleware__DISABLED.ts
Removed previously-disabled middleware stub implementation.
Authentication Tests
packages/core/test/server/authentication-service.test.ts, packages/core/test/server/password-protection/auth-cookie.test.ts
Added comprehensive test suites covering JWT verification, expiry/renewal flows, protection-status gating, cookie security, scope-based domain matching, and localhost/development environment handling.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant Middleware
    participant AuthService
    participant WebOps
    participant Pages

    Client->>Middleware: GET /some-page
    Middleware->>AuthService: authenticateRequest(request)
    
    alt Has valid __fs_auth_token cookie
        AuthService->>AuthService: verifyJWT(token)
        Note over AuthService: JWT verified, storeId matches
        AuthService->>AuthService: Check token.protected & domain scope
        AuthService-->>Middleware: AuthResult (200)
    else JWT expired
        AuthService->>WebOps: POST /renew with expired token
        WebOps-->>AuthService: Renewed token
        AuthService->>AuthService: Set refreshed cookie
        AuthService-->>Middleware: AuthResult (200)
    else No token or verification failed
        AuthService->>WebOps: GET /protection-status
        WebOps-->>AuthService: {protected, scope, token?}
        alt Protected & host in scope
            AuthService->>AuthService: Set cookie (if provided)
            AuthService-->>Middleware: Redirect to /fs-auth-login (307)
        else Not protected or host out of scope
            AuthService->>AuthService: Set cookie (if provided)
            AuthService-->>Middleware: AuthResult (200)
        end
    end
    
    alt AuthResult is 200
        Middleware-->>Pages: Pass through request
        Pages-->>Client: Response
    else Redirect response
        Middleware-->>Client: 307/301/302 Redirect
    end
Loading
sequenceDiagram
    participant User
    participant LoginPage
    participant LoginAPI
    participant WebOps
    participant Middleware

    User->>LoginPage: Navigate to /fs-auth-login?returnTo=/protected
    LoginPage->>User: Render password form
    User->>LoginPage: Submit password
    
    LoginPage->>LoginAPI: POST /api/fs/auth/login {password, returnTo}
    LoginAPI->>WebOps: POST /session {storeId, password}
    alt Valid password
        WebOps-->>LoginAPI: {data.valid: true, data.token}
        LoginAPI->>LoginAPI: Compute safe redirectUrl from returnTo
        LoginAPI->>LoginAPI: Set __fs_auth_token cookie (HttpOnly, Lax, 10min)
        LoginAPI-->>LoginPage: 200 {success: true, redirectUrl}
        LoginPage->>User: window.location.href = redirectUrl
    else Invalid password
        WebOps-->>LoginAPI: 401/403
        LoginAPI-->>LoginPage: 401 {success: false, error: 'Invalid password'}
        LoginPage->>User: Display error
    end
    
    User->>Middleware: Request to /protected with __fs_auth_token
    Middleware->>Middleware: Verify JWT signature & storeId
    Middleware-->>User: Allow request to proceed
Loading

Estimated Code Review Effort

🎯 4 (Complex) | ⏱️ ~60 minutes

The review involves dense authentication logic across multiple modules (JWT verification, session renewal, WebOps integration, cookie security), domain-scope matching, environment-conditional gating, and intricate error/fallback paths. Additionally, the test suite (483 lines) requires careful verification of all auth scenarios, particularly expiry handling and protection-status flows.

Possibly Related PRs

Suggested Reviewers

  • renatomaurovtex
  • lariciamota
  • hellofanny

Poem

🔐 Tokens verified, JWTs signed and sealed,
Passwords hashed, protection's revealed.
Middleware guards the gates with care,
Sessions renewed through the WebOps air. ✨
Login pages bloom, secure and fair!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main feature: adding password protection for stores, which aligns with the comprehensive changeset including authentication service, login page, middleware, and WebOps integration.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/password-protection

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@vlaux vlaux changed the title Password Protected stores feat: password protected stores Apr 15, 2026
@vlaux vlaux force-pushed the feat/password-protection branch from e24ed72 to d4400ee Compare April 15, 2026 17:33
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 11

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/core/src/middleware.ts`:
- Around line 45-55: The redirect branch in middleware.ts drops cookies set
during authentication (from authenticateRequest() / authResult.response), so
when a token is renewed the new Set-Cookie is not forwarded; modify the redirect
handling to detect authResult.response (or any response returned by
authenticateRequest()) and copy its Set-Cookie header(s) into the redirect
response before returning it—e.g., read
authResult.response.headers.getAll('set-cookie') or the single 'set-cookie'
value and append each to the NextResponse.redirect() result (or use
response.cookies.set if available) so the renewed auth cookie is carried over
with the redirect.
- Around line 34-62: Move the authentication call (new
AuthenticationService().authenticateRequest(request)) out of the outer try/catch
so auth errors are not swallowed; if authenticateRequest throws, return a
failing auth response (do not return NextResponse.next()) or let the error
propagate to produce a non-public response. Wrap only the redirect lookup
(redirectsClient.get(path) and subsequent URL/NextResponse.redirect logic) in a
try/catch and on redirect lookup failure fall back to returning
authResult.response; keep references to AuthenticationService,
authenticateRequest, redirectsClient.get, NextResponse.redirect, and
NextResponse.next() to locate the code to change.

In `@packages/core/src/pages/api/fs/auth/login.ts`:
- Around line 43-49: The current login handler treats any non-2xx webopsResponse
as "Invalid password"; update the error handling in the block that checks
webopsResponse (the variable webopsResponse used in the login function) to
inspect webopsResponse.status and only return 401 (or 403 if you prefer) for
explicit auth failures (status === 401 or status === 403) with the existing
invalid-credentials payload, while returning a 503 Service Unavailable (and a
generic upstream error payload) for all other 5xx/non-auth statuses so upstream
outages are not misclassified as bad credentials.
- Around line 53-69: The handler currently echoes the user-supplied returnTo
query into the JSON redirectUrl (variable returnTo) which permits
open-redirects; validate and sanitize returnTo before reflecting it by allowing
only same-site absolute paths (e.g. non-empty string that starts with a single
'/' but not '//' and contains no scheme or host), otherwise fallback to '/';
perform validation where returnTo is computed and use the sanitized value in
response.status(200).json({ redirectUrl: sanitizedReturnTo }), keeping the
existing cookie logic (COOKIE_NAME, TOKEN_TTL_SECONDS,
isSecureAuthCookieForPagesApi, response.setHeader) unchanged.

In `@packages/core/src/pages/fs-auth-login/index.tsx`:
- Around line 1-3: The login page must be marked non-indexable by search
engines; update the FS auth page component in this file to include a robots meta
tag. Import Head from 'next/head' and add <meta name="robots"
content="noindex,nofollow" /> inside the page component's returned JSX (the
default exported component in this module that renders the Button/InputField
form), ensuring the meta tag appears alongside other head elements so crawlers
see it for this public route.

In `@packages/core/src/server/authentication-service.ts`:
- Around line 94-97: The code currently treats a valid token with
verification.payload.protected === false as a long-lived negative cache and
immediately returns NextResponse.next(), which allows visitors to bypass
newly-enabled protection for TOKEN_TTL_SECONDS; modify the logic in
authentication-service.ts (the block handling verification.valid &&
verification.payload) to recheck current protection state instead of trusting a
false protected flag—call the same protection-check routine used for new
requests (or fetch latest merchant protection setting) and only return
NextResponse.next() when both the token is valid and the current protection is
disabled, otherwise proceed to enforce protection (e.g., clear the cookie or
redirect to login). Ensure references to verification.payload.protected,
TOKEN_TTL_SECONDS, and the NextResponse.next() return path are updated
accordingly.
- Around line 114-127: Normalize the Host header before checking for the default
domain: in shouldCheckProtection(request: NextRequest) (and likewise in
shouldProtectDomain(request: NextRequest)) read request.headers.get('host') into
a variable, lower-case it and strip any port portion (e.g.
host.split(':')[0].trim().toLowerCase()), then use that
normalizedHost.endsWith('.vtex.app') instead of the raw header; update both
methods to use the same normalization logic to ensure hosts like
"preview.vtex.app:443" or mixed-case values are handled correctly.
- Around line 268-270: The redirectToLogin method currently sets returnTo to
only request.nextUrl.pathname, losing query parameters; update redirectToLogin
(the method named redirectToLogin in authentication-service.ts) to include the
full path+query by appending request.nextUrl.search (e.g., use
request.nextUrl.pathname + request.nextUrl.search or otherwise serialize the
original nextUrl) when setting loginUrl.searchParams.set('returnTo', ...), so
requests like /search?term=chair&page=2 preserve their query string through the
/fs-auth-login redirect.

In `@packages/core/src/server/password-protection/auth-cookie.ts`:
- Around line 4-9: The isLocalHost function misparses bracketed IPv6 addresses
like "[::1]:3000" because splitting on ':' yields '['; update isLocalHost to
detect and handle bracketed IPv6 first: if host starts with '[' extract the
substring between '[' and ']' as the hostname, otherwise split on ':' and take
the first segment; normalize to lower-case and then check against 'localhost',
'127.0.0.1', '::1' and '[::1]' (or simply strip brackets before comparison) so
IPv6 loopback is correctly recognized and Secure cookies are disabled on
localhost IPv6.

In `@packages/core/src/server/password-protection/webops-api.ts`:
- Around line 16-19: The protectionStatusUrl is built by interpolating
discoveryConfig.api.storeId into the query string which can break for reserved
characters; update the construction of protectionStatusUrl to create the base
URL with origin and pathname, then set the storeId using URL.searchParams (e.g.,
call protectionStatusUrl.searchParams.set('storeId',
discoveryConfig.api.storeId)) so the storeId is properly URL-encoded; locate the
symbol protectionStatusUrl and the referenced discoveryConfig.api.storeId and
replace the current template-string-based URL construction with creation +
searchParams.set.
- Around line 5-9: The code currently assigns origin using the
nullish-coalescing operator which lets an all-whitespace WEBOPS_API_URL become
an empty string and produce an invalid URL; update the logic that computes
origin (the variable named origin and the env var WEBOPS_API_URL) to treat a
trimmed empty string as unset by checking for a non-empty trimmed value (e.g.,
compute a trimmed value into a temp like raw =
process.env.WEBOPS_API_URL?.trim().replaceAll(/\/+$/g, '') and then use origin =
raw && raw.length ? raw : DEFAULT_WEBOPS_ORIGIN) and keep the existing
origin.startsWith('http') normalization to prepend https:// when needed.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 37f3a60c-d19f-41d9-b5df-37e31de47d69

📥 Commits

Reviewing files that changed from the base of the PR and between ea8071b and d4400ee.

⛔ Files ignored due to path filters (2)
  • .gitignore is excluded by none and included by none
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml and included by none
📒 Files selected for processing (12)
  • packages/cli/src/utils/generate.ts
  • packages/core/package.json
  • packages/core/src/middleware.ts
  • packages/core/src/middleware__DISABLED.ts
  • packages/core/src/pages/api/fs/auth/login.ts
  • packages/core/src/pages/fs-auth-login/fs-auth-login.module.scss
  • packages/core/src/pages/fs-auth-login/index.tsx
  • packages/core/src/server/authentication-service.ts
  • packages/core/src/server/password-protection/auth-cookie.ts
  • packages/core/src/server/password-protection/webops-api.ts
  • packages/core/test/server/authentication-service.test.ts
  • packages/core/test/server/password-protection/auth-cookie.test.ts
💤 Files with no reviewable changes (2)
  • packages/cli/src/utils/generate.ts
  • packages/core/src/middleware__DISABLED.ts

Comment on lines +34 to +62
try {
const authService = new AuthenticationService()
const authResult = await authService.authenticateRequest(request)

if (authResult.response.status !== 200) {
return authResult.response
}

if (process.env.ENABLE_REDIRECTS_MIDDLEWARE === 'true') {
const redirect = await redirectsClient.get(path)

if (redirect) {
const redirectUrl = new URL(redirect.to, storeConfig.storeUrl)
const redirectStatusCode = redirect.type === 'permanent' ? 301 : 302
const response = NextResponse.redirect(redirectUrl, redirectStatusCode)

response.headers.set(
'Cache-Control',
'public, max-age=300, stale-while-revalidate=31536000'
)

return response
}
}

return authResult.response
} catch {
return NextResponse.next()
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Don't fail open when auth throws.

The outer catch turns any unexpected auth error into NextResponse.next(), which exposes protected previews instead of keeping the request behind the gate. Keep the auth path outside that fail-open branch, and only soften redirect lookup failures if needed.

🩹 Proposed fix
 export async function middleware(request: NextRequest) {
   const path = request.nextUrl.pathname
+  const authService = new AuthenticationService()
+  const authResult = await authService.authenticateRequest(request)
 
-  try {
-    const authService = new AuthenticationService()
-    const authResult = await authService.authenticateRequest(request)
-
-    if (authResult.response.status !== 200) {
-      return authResult.response
-    }
+  if (authResult.response.status !== 200) {
+    return authResult.response
+  }
 
-    if (process.env.ENABLE_REDIRECTS_MIDDLEWARE === 'true') {
+  if (process.env.ENABLE_REDIRECTS_MIDDLEWARE === 'true') {
+    try {
       const redirect = await redirectsClient.get(path)
 
       if (redirect) {
         const redirectUrl = new URL(redirect.to, storeConfig.storeUrl)
         const redirectStatusCode = redirect.type === 'permanent' ? 301 : 302
@@
 
         return response
       }
+    } catch {
+      return authResult.response
     }
-
-    return authResult.response
-  } catch {
-    return NextResponse.next()
   }
+
+  return authResult.response
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
try {
const authService = new AuthenticationService()
const authResult = await authService.authenticateRequest(request)
if (authResult.response.status !== 200) {
return authResult.response
}
if (process.env.ENABLE_REDIRECTS_MIDDLEWARE === 'true') {
const redirect = await redirectsClient.get(path)
if (redirect) {
const redirectUrl = new URL(redirect.to, storeConfig.storeUrl)
const redirectStatusCode = redirect.type === 'permanent' ? 301 : 302
const response = NextResponse.redirect(redirectUrl, redirectStatusCode)
response.headers.set(
'Cache-Control',
'public, max-age=300, stale-while-revalidate=31536000'
)
return response
}
}
return authResult.response
} catch {
return NextResponse.next()
}
const authService = new AuthenticationService()
const authResult = await authService.authenticateRequest(request)
if (authResult.response.status !== 200) {
return authResult.response
}
if (process.env.ENABLE_REDIRECTS_MIDDLEWARE === 'true') {
try {
const redirect = await redirectsClient.get(path)
if (redirect) {
const redirectUrl = new URL(redirect.to, storeConfig.storeUrl)
const redirectStatusCode = redirect.type === 'permanent' ? 301 : 302
const response = NextResponse.redirect(redirectUrl, redirectStatusCode)
response.headers.set(
'Cache-Control',
'public, max-age=300, stale-while-revalidate=31536000'
)
return response
}
} catch {
return authResult.response
}
}
return authResult.response
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/middleware.ts` around lines 34 - 62, Move the
authentication call (new AuthenticationService().authenticateRequest(request))
out of the outer try/catch so auth errors are not swallowed; if
authenticateRequest throws, return a failing auth response (do not return
NextResponse.next()) or let the error propagate to produce a non-public
response. Wrap only the redirect lookup (redirectsClient.get(path) and
subsequent URL/NextResponse.redirect logic) in a try/catch and on redirect
lookup failure fall back to returning authResult.response; keep references to
AuthenticationService, authenticateRequest, redirectsClient.get,
NextResponse.redirect, and NextResponse.next() to locate the code to change.

Comment thread packages/core/src/middleware.ts
Comment thread packages/core/src/pages/api/fs/auth/login.ts
Comment thread packages/core/src/pages/api/fs/auth/login.ts Outdated
Comment thread packages/core/src/pages/fs-auth-login/index.tsx
Comment thread packages/core/src/server/authentication-service.ts
Comment thread packages/core/src/server/authentication-service.ts Outdated
Comment thread packages/core/src/server/password-protection/auth-cookie.ts
Comment thread packages/core/src/server/password-protection/webops-api.ts Outdated
Comment thread packages/core/src/server/password-protection/webops-api.ts Outdated
@vlaux vlaux force-pushed the feat/password-protection branch from 424bc4c to ef8a078 Compare April 15, 2026 22:48
@vlaux vlaux force-pushed the feat/password-protection branch from fa49b7b to 9c7f6de Compare April 17, 2026 19:06
@vlaux vlaux added enhancement New feature or request contributing Pull request submitted by the community labels Apr 17, 2026
@vlaux vlaux force-pushed the feat/password-protection branch from 2f70225 to 4850045 Compare April 22, 2026 18:57
@vlaux vlaux marked this pull request as ready for review April 23, 2026 11:04
@vlaux vlaux requested a review from a team as a code owner April 23, 2026 11:04
@vlaux vlaux removed the request for review from a team April 23, 2026 11:04
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
packages/core/src/server/authentication-service.ts (2)

96-114: Flatten the valid-token branch — three NextResponse.next() paths collapse into one.

Once a token is verified for this store, every sub-branch (unprotected payload, out-of-scope domain, in-scope protected) returns NextResponse.next(). The intermediate conditionals are dead logic and add cognitive load. As per coding guidelines ("Prefer flat conditionals over nested ifs"), this reads better as an early return.

♻️ Proposed refactor
     if (token) {
       const verification = await this.verifyToken(token)
 
       if (verification.valid && verification.payload) {
-        if (!verification.payload.protected) {
-          return { response: NextResponse.next() }
-        }
-
-        if (!this.shouldProtectDomain(request, verification.payload.scope)) {
-          return { response: NextResponse.next() }
-        }
-
         return { response: NextResponse.next() }
       }
 
       if (verification.expired && verification.payload) {
         return await this.tryRenewSession(request, token, verification.payload)
       }
     }

If those intermediate checks were meant to trigger cookie cleanup or a different response path, the intent isn't visible in the current code — worth either wiring that in or removing the branches.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/server/authentication-service.ts` around lines 96 - 114, In
verifyToken handling inside the authentication flow, collapse the nested
valid-token branches: when verification.valid && verification.payload is true,
immediately return NextResponse.next() instead of checking
verification.payload.protected and shouldProtectDomain(request,
verification.payload.scope) since all three branches currently return the same
value; remove the dead conditionals in the valid branch in
authentication-service.ts (around the verifyToken usage) so the code reads an
early return, and keep the expired-path logic that calls
tryRenewSession(request, token, verification.payload) intact.

161-188: Double as unknown as TokenPayload coerces without validating.

jwtVerify returns JWTPayload, and decodeJwt even more so — nothing guarantees storeId/protected/scope are the expected types at runtime. A malformed token could slip past payloadMatchesStore (e.g. storeId as a number coerced, or protected as a truthy string). Per coding guidelines, prefer runtime validation over type assertions for untrusted inputs.

♻️ Sketch
function toTokenPayload(payload: JWTPayload): TokenPayload | null {
  if (typeof payload.storeId !== 'string') return null
  if (typeof payload.protected !== 'boolean') return null
  if (payload.scope !== undefined && typeof payload.scope !== 'string') return null
  return payload as TokenPayload
}

Then use toTokenPayload(...) in both verifyToken success and expired paths and fail closed when it returns null.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/server/authentication-service.ts` around lines 161 - 188,
The code currently coerces untrusted JWT payloads with "as unknown as
TokenPayload"; add a runtime validator (e.g., toTokenPayload(payload:
JWTPayload): TokenPayload | null) that checks typeof payload.storeId ===
'string', typeof payload.protected === 'boolean', and payload.scope is undefined
or a string, then use that validator in both the jwtVerify success path and the
decodeJwt expired path (replace the double casts) and return failure (valid:
false) when the validator returns null; reference jwtVerify, decodeJwt,
isJwtExpiredError, and payloadMatchesStore to locate where to plug the validator
and to keep the existing store-matching logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/core/src/server/authentication-service.ts`:
- Around line 84-87: getNormalizedHost currently splits the host on ':' which
breaks for bracketed IPv6 like "[::1]:8080"; update
getNormalizedHost(NextRequest) to handle bracketed IPv6 by detecting if hostname
startsWith('[') and, in that case, extract the text between the first '[' and
the matching ']' (dropping any trailing :port), otherwise strip the port by
cutting at the last ':' (use lastIndexOf) to avoid splitting hostnames with
colons; then trim and toLowerCase the result so downstream checks (e.g.,
endsWith('.vtex.app')) work correctly.

---

Nitpick comments:
In `@packages/core/src/server/authentication-service.ts`:
- Around line 96-114: In verifyToken handling inside the authentication flow,
collapse the nested valid-token branches: when verification.valid &&
verification.payload is true, immediately return NextResponse.next() instead of
checking verification.payload.protected and shouldProtectDomain(request,
verification.payload.scope) since all three branches currently return the same
value; remove the dead conditionals in the valid branch in
authentication-service.ts (around the verifyToken usage) so the code reads an
early return, and keep the expired-path logic that calls
tryRenewSession(request, token, verification.payload) intact.
- Around line 161-188: The code currently coerces untrusted JWT payloads with
"as unknown as TokenPayload"; add a runtime validator (e.g.,
toTokenPayload(payload: JWTPayload): TokenPayload | null) that checks typeof
payload.storeId === 'string', typeof payload.protected === 'boolean', and
payload.scope is undefined or a string, then use that validator in both the
jwtVerify success path and the decodeJwt expired path (replace the double casts)
and return failure (valid: false) when the validator returns null; reference
jwtVerify, decodeJwt, isJwtExpiredError, and payloadMatchesStore to locate where
to plug the validator and to keep the existing store-matching logic.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 74cf2e96-ce7f-4ae5-8e30-91aab1c3c32f

📥 Commits

Reviewing files that changed from the base of the PR and between d4400ee and 4850045.

📒 Files selected for processing (6)
  • packages/core/src/middleware.ts
  • packages/core/src/pages/api/fs/auth/login.ts
  • packages/core/src/pages/fs-auth-login/index.tsx
  • packages/core/src/server/authentication-service.ts
  • packages/core/src/server/password-protection/auth-cookie.ts
  • packages/core/src/server/password-protection/webops-api.ts
✅ Files skipped from review due to trivial changes (1)
  • packages/core/src/middleware.ts
🚧 Files skipped from review as they are similar to previous changes (3)
  • packages/core/src/pages/fs-auth-login/index.tsx
  • packages/core/src/server/password-protection/auth-cookie.ts
  • packages/core/src/server/password-protection/webops-api.ts

Comment on lines +84 to +87
private getNormalizedHost(request: NextRequest): string {
const hostname = request.headers.get('host') || ''
return hostname.split(':')[0].trim().toLowerCase()
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

getNormalizedHost still breaks on bracketed IPv6 literals.

"[::1]:8080".split(':')[0] returns "[", so the .endsWith('.vtex.app') / scope checks produce incorrect results for IPv6 hosts. Given the commit history calls out "bracketed IPv6 handling," this edge case likely deserves explicit coverage.

🩹 Proposed fix
   private getNormalizedHost(request: NextRequest): string {
-    const hostname = request.headers.get('host') || ''
-    return hostname.split(':')[0].trim().toLowerCase()
+    const hostname = (request.headers.get('host') || '').trim().toLowerCase()
+
+    // Bracketed IPv6: "[::1]:8080" -> "[::1]"
+    if (hostname.startsWith('[')) {
+      const end = hostname.indexOf(']')
+      return end === -1 ? hostname : hostname.slice(0, end + 1)
+    }
+
+    // IPv4 / regular hostnames: strip ":port"
+    return hostname.split(':')[0]
   }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/server/authentication-service.ts` around lines 84 - 87,
getNormalizedHost currently splits the host on ':' which breaks for bracketed
IPv6 like "[::1]:8080"; update getNormalizedHost(NextRequest) to handle
bracketed IPv6 by detecting if hostname startsWith('[') and, in that case,
extract the text between the first '[' and the matching ']' (dropping any
trailing :port), otherwise strip the port by cutting at the last ':' (use
lastIndexOf) to avoid splitting hostnames with colons; then trim and toLowerCase
the result so downstream checks (e.g., endsWith('.vtex.app')) work correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contributing Pull request submitted by the community enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant